Skip to content

fix(consensus): exit non-zero on transient remote-signer startup failure#158

Open
tomasztrzos wants to merge 2 commits into
circlefin:mainfrom
tomasztrzos:fix/remote-signer-startup-restart
Open

fix(consensus): exit non-zero on transient remote-signer startup failure#158
tomasztrzos wants to merge 2 commits into
circlefin:mainfrom
tomasztrzos:fix/remote-signer-startup-restart

Conversation

@tomasztrzos

Copy link
Copy Markdown

When the remote signer is unreachable at startup, the node's eager connect() fails fast (unlike the runtime signing path, which retries via sign_message_with_retry). run() caught this in its startup error arm and called wait_for_termination(), parking the process alive-but-idle until SIGTERM. A supervisor's restart policy never fires and a process-only liveness check doesn't notice, so the node stays parked indefinitely even after the signer recovers seconds later.

Return an error for transient remote-signer failures (Transport, ServiceUnavailable, Timeout) so main exits non-zero and the supervisor restarts the node — the same behavior already used for the EL IPC watchdog. Permanent errors (configuration, authentication, malformed responses) still park for manual intervention.

gRPC Status errors are intentionally excluded: the failure mode this guards against is the eager connect() at startup, which surfaces as Transport.

Add unit tests covering the helper through the production eyre wrap_err chain.

When the remote signer is unreachable at startup, the node's eager
connect() fails fast (unlike the runtime signing path, which retries via
sign_message_with_retry). run() caught this in its startup error arm and
called wait_for_termination(), parking the process alive-but-idle until
SIGTERM. A supervisor's restart policy never fires and a process-only
liveness check doesn't notice, so the node stays parked indefinitely even
after the signer recovers seconds later.

Return an error for transient remote-signer failures (Transport,
ServiceUnavailable, Timeout) so main exits non-zero and the supervisor
restarts the node — the same behavior already used for the EL IPC
watchdog. Permanent errors (configuration, authentication, malformed
responses) still park for manual intervention.

gRPC Status errors are intentionally excluded: the failure mode this
guards against is the eager connect() at startup, which surfaces as
Transport.

Add unit tests covering the helper through the production eyre wrap_err
chain.

@romac romac left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR, sounds totally reasonable to me to crash on connectivity issues for improved signaling/recovery 👍

@romac romac left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After another round of review, I just realized we need to preserve a couple more errors within the error trace, otherwise these two cases will preserve the alive-but-idle failure mode this PR is trying to remove.

Please find attached a tentative diff for fixing that.

Show suggested changes
diff --git a/Cargo.lock b/Cargo.lock
index 47d70219..648750e9 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1820,6 +1820,7 @@ dependencies = [
  "tokio",
  "tokio-tungstenite 0.26.2",
  "tokio-util",
+ "tonic 0.12.3",
  "tower 0.5.3",
  "tracing",
  "url",
diff --git a/crates/malachite-app/Cargo.toml b/crates/malachite-app/Cargo.toml
index a827a4cd..8a3c796f 100644
--- a/crates/malachite-app/Cargo.toml
+++ b/crates/malachite-app/Cargo.toml
@@ -89,6 +89,7 @@ reqwest = { workspace = true, features = ["json"] }
 serial_test = { workspace = true }
 tempfile = { workspace = true }
 tokio = { workspace = true, features = ["test-util"] }
+tonic.workspace = true
 tower = { workspace = true }
 
 [lints]
diff --git a/crates/malachite-app/src/node.rs b/crates/malachite-app/src/node.rs
index c9b3e103..a7769434 100644
--- a/crates/malachite-app/src/node.rs
+++ b/crates/malachite-app/src/node.rs
@@ -894,21 +894,13 @@ fn install_sigterm_handler(_handle: &Handle) {}
 /// authentication, malformed responses) return `false` so they still park for
 /// manual intervention rather than restart-looping.
 ///
-/// Note: gRPC `Status` errors (`RemoteSigningError::Status`) are intentionally out
-/// of scope here — the failure this guards against is the eager `connect()` at
-/// startup, which surfaces as `Transport`. Keeping the set narrow avoids
-/// restart-looping on errors whose transience is ambiguous.
+/// The exact retryability rules live with `RemoteSigningError`, where tonic status
+/// codes and retry-exhaustion sources can be classified without string matching.
 fn is_retryable_startup_error(error: &eyre::Report) -> bool {
-    error.chain().any(|cause| {
-        matches!(
-            cause.downcast_ref::<RemoteSigningError>(),
-            Some(
-                RemoteSigningError::Transport(_)
-                    | RemoteSigningError::ServiceUnavailable(_)
-                    | RemoteSigningError::Timeout(_)
-            )
-        )
-    })
+    error
+        .chain()
+        .filter_map(|cause| cause.downcast_ref::<RemoteSigningError>())
+        .any(RemoteSigningError::is_retryable_startup_error)
 }
 
 /// Wait for a termination signal (SIGTERM on Unix)
@@ -1049,6 +1041,14 @@ mod tests {
             .wrap_err("Node failed to start")
     }
 
+    fn wrapped_validator_proof_signing(err: RemoteSigningError) -> eyre::Report {
+        let signing_error = arc_consensus_types::signing::SigningError::from_source(err);
+        eyre::Report::new(signing_error)
+            .wrap_err("Failed to sign validator proof")
+            .wrap_err("Failed to create network identity with validator proof")
+            .wrap_err("Node failed to start")
+    }
+
     #[test]
     fn transient_remote_signer_errors_are_retryable() {
         assert!(is_retryable_startup_error(&wrapped(
@@ -1057,6 +1057,22 @@ mod tests {
         assert!(is_retryable_startup_error(&wrapped(
             RemoteSigningError::Timeout("slow".into())
         )));
+        assert!(is_retryable_startup_error(&wrapped(
+            RemoteSigningError::Status(Box::new(tonic::Status::unavailable("signer warming up")))
+        )));
+        assert!(is_retryable_startup_error(&wrapped(
+            RemoteSigningError::Status(Box::new(tonic::Status::deadline_exceeded(
+                "signer request timed out"
+            )))
+        )));
+        assert!(is_retryable_startup_error(
+            &wrapped_validator_proof_signing(RemoteSigningError::RetryExhausted {
+                retries: 3,
+                source: Box::new(RemoteSigningError::Status(Box::new(
+                    tonic::Status::unavailable("signer warming up")
+                ))),
+            })
+        ));
     }
 
     #[test]
@@ -1067,6 +1083,17 @@ mod tests {
         assert!(!is_retryable_startup_error(&wrapped(
             RemoteSigningError::Authentication("denied".into())
         )));
+        assert!(!is_retryable_startup_error(&wrapped(
+            RemoteSigningError::Status(Box::new(tonic::Status::unauthenticated("bad credentials")))
+        )));
+        assert!(!is_retryable_startup_error(
+            &wrapped_validator_proof_signing(RemoteSigningError::RetryExhausted {
+                retries: 3,
+                source: Box::new(RemoteSigningError::Status(Box::new(
+                    tonic::Status::unauthenticated("bad credentials")
+                ))),
+            })
+        ));
     }
 
     #[test]
diff --git a/crates/malachite-app/src/validator_proof.rs b/crates/malachite-app/src/validator_proof.rs
index 6fc755ff..11b9317b 100644
--- a/crates/malachite-app/src/validator_proof.rs
+++ b/crates/malachite-app/src/validator_proof.rs
@@ -25,7 +25,7 @@ use arc_consensus_types::codec::{network::NetCodec, Codec};
 use arc_consensus_types::signing::Signer;
 use arc_signer::ArcSigningProvider;
 use bytes::Bytes;
-use eyre::Result;
+use eyre::{Result, WrapErr};
 use tracing::info;
 
 /// Create a signed validator proof binding the consensus public key to the P2P peer ID.
@@ -50,7 +50,8 @@ pub async fn create_validator_proof(
     let proof = signing_provider
         .sign_validator_proof(public_key_bytes, peer_id_bytes)
         .await
-        .map_err(|e| eyre::eyre!("Failed to sign validator proof: {e}"))?;
+        .map_err(eyre::Report::new)
+        .wrap_err("Failed to sign validator proof")?;
 
     let proof_bytes = NetCodec
         .encode(&proof)
diff --git a/crates/remote-signer/src/client.rs b/crates/remote-signer/src/client.rs
index f9dbd0a7..18985d9c 100644
--- a/crates/remote-signer/src/client.rs
+++ b/crates/remote-signer/src/client.rs
@@ -209,6 +209,7 @@ impl RemoteSignerClient {
 
                 Err(RemoteSigningError::RetryExhausted {
                     retries: config.retry_config.max_retries,
+                    source: Box::new(e),
                 })
             }
         }
@@ -389,7 +390,7 @@ mod integration_tests {
                 "Signing should fail with bad endpoint"
             );
 
-            if let Err(RemoteSigningError::RetryExhausted { retries }) = sign_result {
+            if let Err(RemoteSigningError::RetryExhausted { retries, .. }) = sign_result {
                 assert_eq!(retries, 2, "Should have exhausted exactly 2 retries");
             }
         }
diff --git a/crates/remote-signer/src/error.rs b/crates/remote-signer/src/error.rs
index 7d9f7a28..b5faf4d1 100644
--- a/crates/remote-signer/src/error.rs
+++ b/crates/remote-signer/src/error.rs
@@ -37,7 +37,11 @@ pub enum RemoteSigningError {
     Timeout(String),
 
     #[error("Retry exhausted after {retries} attempts")]
-    RetryExhausted { retries: usize },
+    RetryExhausted {
+        retries: usize,
+        #[source]
+        source: Box<RemoteSigningError>,
+    },
 
     #[error("Configuration error: {0}")]
     Configuration(String),
@@ -45,3 +49,19 @@ pub enum RemoteSigningError {
     #[error("Service unavailable: {0}")]
     ServiceUnavailable(String),
 }
+
+impl RemoteSigningError {
+    /// Returns `true` if this error represents a transient startup failure that a
+    /// process restart is likely to recover from.
+    pub fn is_retryable_startup_error(&self) -> bool {
+        match self {
+            Self::Transport(_) | Self::ServiceUnavailable(_) | Self::Timeout(_) => true,
+            Self::Status(status) => matches!(
+                status.code(),
+                tonic::Code::Unavailable | tonic::Code::DeadlineExceeded
+            ),
+            Self::RetryExhausted { source, .. } => source.is_retryable_startup_error(),
+            Self::Authentication(_) | Self::Configuration(_) | Self::InvalidResponse(_) => false,
+        }
+    }
+}

Address review: two startup signing paths reintroduced the alive-but-idle
failure mode because their transient cause was lost before reaching
is_retryable_startup_error.

- validator_proof signing stringified the error via eyre::eyre!, breaking
  the downcast; wrap it with eyre::Report::new + wrap_err instead.
- RetryExhausted dropped its underlying cause; carry it as #[source].
- Move retryability classification into RemoteSigningError, covering
  transient gRPC Status codes (Unavailable, DeadlineExceeded) and
  recursing through RetryExhausted.
@tomasztrzos
tomasztrzos requested a review from romac June 17, 2026 09:15
@ZhiyuCircle ZhiyuCircle added bug Something isn't working CL Component: CL labels Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working CL Component: CL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants